Skip to content

fix(workflows): escape the literal bracket in workflow resolve layer output - #3882

Closed
jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix/overlay-resolve-tier-markup
Closed

fix(workflows): escape the literal bracket in workflow resolve layer output#3882
jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix/overlay-resolve-tier-markup

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Problem

workflow_resolve in src/specify_cli/workflows/overlays/_commands.py prints each layer like this:

f"  • [{layer.tier}] {layer.source} (priority={priority})"

Rich parses [base] and [project-overlay] as style tags, so the tier column is silently swallowed — on every single run, not just for odd input.

Reproduction on current main (81bf741)

BEFORE: '*  .specify/workflows/wf/workflow.yml (priority=n/a)'
AFTER : '* [base] .specify/workflows/wf/workflow.yml (priority=n/a)'

BEFORE: '*  ov1.yml (priority=10)'
AFTER : '* [project-overlay] ov1.yml (priority=10)'

Note the double space in the BEFORE lines — that is where the tier used to be. The layer's tier is one of the two things workflow resolve exists to tell you (which layer, at what precedence), so the human-readable output is missing half its answer. The --json payload carries tier correctly; only the printed form loses it.

Why the existing test didn't catch it

test_workflow_resolve asserts:

assert "base" in result.output

That passes with the bug, because the step-attribution section prints the same word as a step source. The tier has to be asserted in bracket form, on the layers line itself — which is what the new test does.

Fix

Escape the literal bracket with \[, exactly as presets/_commands.py:378 already does:

# presets/_commands.py:374-378
# Escape the literal bracket (\[) so Rich renders `[<strategy>]`
f"    {i + 1}. \[{_escape_markup(str(strategy_label))}] "

I also escaped the interpolated values in the same output block — layer.source is a filesystem path and composed.step_id / composed.source are user-authored ids, all of which can contain brackets and would break the same lines. That keeps the whole of this one command's output correct rather than fixing half of it.

No breaking change. rich.markup.escape is a no-op on bracket-free values, and the --json payload is untouched. The only difference is that output which previously lost characters now shows them.

Verification

  • Fail-before / pass-after: the new test fails on unpatched src and passes with the fix. File: 1 failed → 21 passed.
  • Scoped regression over tests/workflows: failure set identical to the clean-main baseline captured on 81bf741 (10 pre-existing in scope, all Windows symlink-privilege).
  • uvx ruff@0.15.0 check src tests → clean

Written with assistance from Claude Code. Bug found, reproduced, and verified by me on current main.

`workflow_resolve` printed each layer as

    f"  • [{layer.tier}] {layer.source} (priority={priority})"

Rich parses `[base]` and `[project-overlay]` as style tags, so the tier
column was silently swallowed on every run:

    BEFORE: '*  .specify/workflows/wf/workflow.yml (priority=n/a)'
    AFTER : '* [base] .specify/workflows/wf/workflow.yml (priority=n/a)'

The tier is one of the two things the command exists to report, and the
JSON payload still carried it correctly — only the human-readable output
lost it.

The existing test asserts `"base" in result.output`, which passes anyway
because the step-attribution section prints the same word as a source, so
the missing column was masked.

Escape the literal bracket with `\[`, matching presets/_commands.py:378,
and escape the user-controlled layer sources and step ids while there.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes Rich rendering so workflow resolution displays layer tiers and bracket-containing identifiers correctly.

Changes:

  • Escapes literal brackets and interpolated values in resolution output.
  • Adds regression coverage for tier labels.
Show a summary per file
File Description
src/specify_cli/workflows/overlays/_commands.py Escapes Rich markup in layer and attribution output.
tests/workflows/test_overlay_commands.py Verifies tier labels remain visible.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread src/specify_cli/workflows/overlays/_commands.py
@jawwad-ali

Copy link
Copy Markdown
Contributor Author

Closing this as superseded184de79 on main (#3879) already landed the same fix, and landed it slightly better than mine.

Main's version does everything this PR does:

console.print(
    f"  • \[{_escape_markup(layer.tier)}] "
    f"{_escape_markup(layer.source)} "
    f"(priority={priority})"
)
...
console.print(
    f"  • {_escape_markup(composed.step_id)}: "
    f"{_escape_markup(composed.source)}"
)

…including the step-attribution escaping, and it ships test_workflow_resolve_prints_tier_labels asserting [base] and [project-overlay] reach the output. I diffed both versions — there is nothing here that main lacks.

It also answers the review comment on this PR better than I could have: its comment records that step IDs "come from base-workflow / overlay YAML, which only bans : — brackets pass validation", which is the reachability argument the review was asking me to pin with a test. So that concern is already addressed on main.

No point carrying a duplicate through review. Thanks for the look regardless — the observation that my test left the step-ID escaping unprotected was correct, and I've applied the same "does removing this line still pass?" check to the other PRs in this batch.

@jawwad-ali jawwad-ali closed this Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants